-
Notifications
You must be signed in to change notification settings - Fork 32
transfers: read various filter params in http request #474
Conversation
query := `select transfer_id from transfers where user_id = ? and deleted_at is null` | ||
func (r *SQLRepo) getUserTransfers(userID id.User, params transferFilterParams) ([]*model.Transfer, error) { | ||
query := `select transfer_id from transfers | ||
where user_id = ? and created_at >= ? and created_at <= ? and deleted_at is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an index for created_at
thats desc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that its ignore me lol doesn't apply here as I was thinking more about it,X or equal to
Is it fine to get duplicates if they query this by sequential dates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no index on created_at desc
for transfers. We should add one... Thanks for the catch
stmt, err := r.db.Prepare(query) | ||
if err != nil { | ||
return nil, err | ||
} | ||
defer stmt.Close() | ||
|
||
rows, err := stmt.Query(userID) | ||
rows, err := stmt.Query(userID, params.StartDate, params.EndDate, params.Limit, params.Offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we query is params.EndDate
time segment 23:59:999.000
? I could have missed it but didn't see anything below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's now() + "24 hours"
basically. We could bump it forward more.
EndDate: time.Now().Add(24 * time.Hour),
In the future we need to support #447 (scheduled transfers) where I think this endpoint should order by scheduled_at or created_at (coalesce the two) because transfers without scheduled_at are to be uploaded/posted on the current day.
internal/transfers/transfers.go
Outdated
fmt.Printf("params.EndDate=%v\n", params.EndDate) | ||
} | ||
if v := r.URL.Query().Get("limit"); v != "" { | ||
params.Limit, _ = strconv.ParseInt(v, 10, 32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to wrap these with a hard limit so we don't page tons and tons of objects back.
Codecov Report
@@ Coverage Diff @@
## master #474 +/- ##
==========================================
+ Coverage 58.33% 58.58% +0.24%
==========================================
Files 112 114 +2
Lines 8302 8347 +45
==========================================
+ Hits 4843 4890 +47
Misses 2735 2735
+ Partials 724 722 -2
Continue to review full report at Codecov.
|
Fixes: #455